- 
                Notifications
    You must be signed in to change notification settings 
- Fork 279
          Remove duplicate remove_outer_derefs and refactor mod deref
          #573
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: tests.refs.fields
Are you sure you want to change the base?
  
    Remove duplicate remove_outer_derefs and refactor mod deref
  
  #573
              Conversation
…in terms of `try_remove_outer_deref_as_ref` so we know they're in sync.
…and this lets us eliminate duplicate calls.
remove_outer_derefs and refactor mod deref
      …ng `try_remove_outer_deref`.
It encouraged an anti-pattern anyways, using `has_outer_deref` and then `remove_outer_deref` instead of `try_remove_outer_deref`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in favor of combining the match arms. I do however like your original idea outlined here in order to bind source in the match arm for use in the body and would support that change.
| Rvalue::AddressOf(_, p) => { | ||
| // Instrument which local's address is taken | ||
| self.loc(location.successor_within_block(), addr_local_fn) | ||
| let source = try_remove_outer_deref(*p, self.tcx()) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't want to combine these, there are more complicated cases incoming and it'll be much clearer if they're separate
| // this is a reborrow or field reference, i.e. _2 = &(*_1) | ||
| let source = remove_outer_deref(*p, self.tcx()); | ||
| if let BorrowKind::Mut { .. } = bkind { | ||
| let source = try_remove_outer_deref(*p, self.tcx()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't want to combine these, there are more complicated cases incoming and it'll be much clearer if they're separate
| 
 My original idea uses  | 
| 
 | 
| I understand there might be more complex logic coming, so I shouldn't combine the logic in the way I did, but I don't think combining  | 
This refactors
mod deref, implementing most of the functions there in terms of onetry_remove_outer_deref_as_refso that we know they're all in sync. Also,try_remove_outer_derefis added so that you don't have to do separatehas_outer_derefandremove_outer_derefcalls.Then I used this to de-duplicate the 1
has_outer_refand 2remove_outer_derefcalls in theRValue::{AddressOf,Ref}matcharms by combining the arms and usingtry_remove_outer_deref.Then I used
applyto simplify the conditional builder logic. It's quite a small but useful dependency for functional, chaining code.The remaining code is still quite similar between the
RValue::AddressOfandRvalue::Refmatcharms, so maybe we can refactor that into common code as well.